Skip to content

feat(server): echo request headers in response over REST transport#1509

Merged
parthea merged 1 commit intomainfrom
echo-headers
Apr 29, 2024
Merged

feat(server): echo request headers in response over REST transport#1509
parthea merged 1 commit intomainfrom
echo-headers

Conversation

@vchudnov-g
Copy link
Contributor

@vchudnov-g vchudnov-g commented Apr 26, 2024 •
edited
Loading

Towards #1493, but note that this will occur for all Showcase services, not just Echo.

This can be verified manually on the shell via:

❯ curl -i -X POST -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' http://localhost:7469/v1beta1/echo:echo -d'{"content":"g
reetings"}'                                                                                                                                                 
HTTP/1.1 200 OK
X-Showcase-Request-Accept: */*
X-Showcase-Request-Content-Length: 23
X-Showcase-Request-Content-Type: application/json
X-Showcase-Request-User-Agent: curl/8.5.0
X-Showcase-Request-X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0
Date: Fri, 26 Apr 2024 21:33:24 GMT
Content-Length: 104
Content-Type: text/plain; charset=utf-8

{
  "content":  "greetings",
  "severity":  "UNNECESSARY",
  "requestId":  "",
  "otherRequestId":  ""
}

and works even for error responses:

❯ curl -i -X POST -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' http://localhost:7469/v1beta1/echo:echo -d'{"foo":"greet
ings"}'                                                                                                                                                     
HTTP/1.1 400 Bad Request
X-Showcase-Request-Accept: */*
X-Showcase-Request-Content-Length: 19
X-Showcase-Request-Content-Type: application/json
X-Showcase-Request-User-Agent: curl/8.5.0
X-Showcase-Request-X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0
Date: Fri, 26 Apr 2024 21:47:05 GMT
Content-Length: 160
Content-Type: text/plain; charset=utf-8

{"error":{"code":400,"message":"error reading body params '*': proto: (line 1:2): unknown field \"foo\"","details":null,"Body":"","Header":null,"Errors":null}

@vchudnov-g vchudnov-g requested review from a team, noahdietz and parthea April 26, 2024 21:40
@vchudnov-g vchudnov-g changed the title feat: echo request headers in response over REST transport feat(server): echo request headers in response over REST transport Apr 26, 2024
parthea
parthea approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I tested this locally using the code in googleapis/gapic-generator-python#1999

@parthea parthea merged commit de93c4c into main Apr 29, 2024
@parthea parthea deleted the echo-headers branch April 29, 2024 22:04
@release-please release-please bot mentioned this pull request Apr 29, 2024
Merged
@noahdietz
Copy link
Contributor

noahdietz commented Apr 29, 2024

FWIW I don't think @vchudnov-g had landed on a header prefix yet, at least the suggestions in the document he wrote are different than what is implemented here...

@parthea
Copy link
Contributor

parthea commented Apr 29, 2024

At the time of merging this PR it seemed like we had settled on a prefix based on the comment thread in the document.

@noahdietz
Copy link
Contributor

noahdietz commented Apr 29, 2024

At the time of merging this PR it seemed like we had settled on a prefix based on the comment thread in the document.

You're right, @vchudnov-g had settled on that!

@vchudnov-g
Copy link
Contributor Author

vchudnov-g commented May 1, 2024

One thought that occurs to me: maybe for the same reasons we have prefixes in the echoed http headers, we should also have them in the gRPC trailers that echo the gRPC request headers? Thoughts?

@noahdietz
Copy link
Contributor

noahdietz commented May 1, 2024

One thought that occurs to me: maybe for the same reasons we have prefixes in the echoed http headers, we should also have them in the gRPC trailers that echo the gRPC request headers? Thoughts?

Yeah it probably makes sense to do, but let's make sure we blast it out in release notes for those that already have trailer tests.

@parthea parthea mentioned this pull request Mar 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parthea parthea parthea approved these changes

@noahdietz noahdietz Awaiting requested review from noahdietz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vchudnov-g @noahdietz @parthea